Use undef for uninitialized bytes in constants #83698

erikdesjardins · 2021-03-31T05:35:56Z

This generates good code when the const is fully uninit, e.g.

#[no_mangle]
pub const fn fully_uninit() -> MaybeUninit<[u8; 10]> {
    const M: MaybeUninit<[u8; 10]> = MaybeUninit::uninit();
    M
}

generates

fully_uninit:
	ret

as you would expect.

There is no improvement, however, when it's partially uninit, e.g.

pub struct PartiallyUninit {
    x: u64,
    y: MaybeUninit<[u8; 10]>
}

#[no_mangle]
pub const fn partially_uninit() -> PartiallyUninit {
    const X: PartiallyUninit = PartiallyUninit { x: 0xdeadbeefcafe, y: MaybeUninit::uninit() };
    X
}

generates

partially_uninit:
	mov	rax, rdi
	mov	rcx, qword ptr [rip + .L__unnamed_1+16]
	mov	qword ptr [rdi + 16], rcx
	movups	xmm0, xmmword ptr [rip + .L__unnamed_1]
	movups	xmmword ptr [rdi], xmm0
	ret

.L__unnamed_1:
	.asciz	"\376\312\357\276\255\336\000"
	.zero	16
	.size	.L__unnamed_1, 24

which copies a bunch of zeros in place of the undef bytes, the same as before this change.

Edit: generating partially-undef constants isn't viable at the moment anyways due to #84565, so it's disabled

rust-highfive · 2021-03-31T05:35:59Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

oli-obk · 2021-03-31T10:43:13Z

r? @oli-obk

compiler/rustc_middle/src/mir/interpret/allocation.rs

oli-obk · 2021-03-31T10:53:26Z

cc @rust-lang/wg-llvm we're now marking undef bytes from constants as undef, just like we mark padding in constructed values.

oli-obk · 2021-04-01T08:59:01Z

@bors r+

bors · 2021-04-01T08:59:02Z

📌 Commit be66c2e has been approved by oli-obk

Use undef for uninitialized bytes in constants Fixes rust-lang#83657 This generates good code when the const is fully uninit, e.g. ```rust #[no_mangle] pub const fn fully_uninit() -> MaybeUninit<[u8; 10]> { const M: MaybeUninit<[u8; 10]> = MaybeUninit::uninit(); M } ``` generates ```asm fully_uninit: ret ``` as you would expect. There is no improvement, however, when it's partially uninit, e.g. ```rust pub struct PartiallyUninit { x: u64, y: MaybeUninit<[u8; 10]> } #[no_mangle] pub const fn partially_uninit() -> PartiallyUninit { const X: PartiallyUninit = PartiallyUninit { x: 0xdeadbeefcafe, y: MaybeUninit::uninit() }; X } ``` generates ```asm partially_uninit: mov rax, rdi mov rcx, qword ptr [rip + .L__unnamed_1+16] mov qword ptr [rdi + 16], rcx movups xmm0, xmmword ptr [rip + .L__unnamed_1] movups xmmword ptr [rdi], xmm0 ret .L__unnamed_1: .asciz "\376\312\357\276\255\336\000" .zero 16 .size .L__unnamed_1, 24 ``` which copies a bunch of zeros in place of the undef bytes, the same as before this change. The constant is undef where it should be, though (see the added test) so this seems to be a missed optimization in LLVM.

erikdesjardins · 2021-04-02T16:52:38Z

I don't think this should be rolled up, it could affect perf or expose existing UB (because previously reading uninitialized bytes from constants at runtime would "work")

Use undef for uninitialized bytes in constants Fixes rust-lang#83657 This generates good code when the const is fully uninit, e.g. ```rust #[no_mangle] pub const fn fully_uninit() -> MaybeUninit<[u8; 10]> { const M: MaybeUninit<[u8; 10]> = MaybeUninit::uninit(); M } ``` generates ```asm fully_uninit: ret ``` as you would expect. There is no improvement, however, when it's partially uninit, e.g. ```rust pub struct PartiallyUninit { x: u64, y: MaybeUninit<[u8; 10]> } #[no_mangle] pub const fn partially_uninit() -> PartiallyUninit { const X: PartiallyUninit = PartiallyUninit { x: 0xdeadbeefcafe, y: MaybeUninit::uninit() }; X } ``` generates ```asm partially_uninit: mov rax, rdi mov rcx, qword ptr [rip + .L__unnamed_1+16] mov qword ptr [rdi + 16], rcx movups xmm0, xmmword ptr [rip + .L__unnamed_1] movups xmmword ptr [rdi], xmm0 ret .L__unnamed_1: .asciz "\376\312\357\276\255\336\000" .zero 16 .size .L__unnamed_1, 24 ``` which copies a bunch of zeros in place of the undef bytes, the same as before this change. The constant is undef where it should be, though (see the added test) so this seems to be a missed optimization in LLVM.

RalfJung · 2021-04-02T17:16:13Z

@bors rollup=never

@Dylan-DPC might be better to re-roll that rollup... sorry for this.

bors · 2021-04-02T22:47:23Z

⌛ Testing commit be66c2e with merge 24f957fcbe9f9432f0c74cc707a5f008268f4984...

bors · 2021-04-02T23:11:49Z

💔 Test failed - checks-actions

erikdesjardins · 2021-04-06T03:34:15Z

wc -c < /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small/foo.wasm
1139
[ "`wc -c < /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small/foo.wasm`" -lt "1024" ]

When building this test locally, foo.wasm is never below 1024 bytes (which is strange), but there is a regression: on master foo.wasm is 6k, and with these changes it's 140k (!). It seems to be defeating some optimization in LLVM, so we're no longer able to eliminate a bunch of formatting code. I'm trying to figure out why.

It may be worth a perf run to see if this change as-is causes widespread regressions (I suspect it will), or if that wasm test is just fragile for some reason

nagisa · 2021-04-06T04:38:11Z

Now that you mention this, In my memory llvm would fail to put constants with uninit in them into bss sections in the past. Does wasm have something similar?

…

On Tue, 6 Apr 2021, 06:34 erikdesjardins, ***@***.***> wrote: wc -c < /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small/foo.wasm 1139 [ "`wc -c < /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/wasm-panic-small/wasm-panic-small/foo.wasm`" -lt "1024" ] When building this test locally, it's never below 1024 bytes (which is strange), but there is a regression: at HEAD~2 foo.wasm is 6k, and with these changes it's 140k (!). It seems to be defeating some optimization in LLVM, so we're no longer able to eliminate a bunch of formatting code. I'm trying to figure out why. It may be worth a perf run to see if this change as-is causes widespread regressions (I suspect it will), or if that wasm test is just fragile for some reason — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#83698 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFFZUWSLBZABFD6GSH6RF3THJ6MNANCNFSM42DX4N6A> .

RalfJung · 2021-04-06T07:58:53Z

@bors try @rust-timer queue

oli-obk · 2021-08-26T08:28:51Z

@bors r=RalfJung,oli-obk

bors · 2021-08-26T08:28:52Z

📌 Commit adf3b01 has been approved by RalfJung,oli-obk

bors · 2021-08-26T10:49:29Z

⌛ Testing commit adf3b01 with merge 20997f6...

bors · 2021-08-26T13:30:07Z

☀️ Test successful - checks-actions
Approved by: RalfJung,oli-obk
Pushing 20997f6 to master...

the8472 · 2021-09-17T22:37:13Z

While the perf results show improvements in instructions counts this looks like a significant (non-noise) regression in wall times.

The jump is noticable in the timelines of encodings-opt or ~~await-call-tree-opt~~ helloworld-opt

Labeling as regression so that the perf team can have a look at it.

oli-obk · 2021-09-18T07:30:01Z

the await call tree opt change is in #88308 not in this PR

there are a few other noticeable regressions in the timelines though, yes.

Most of the non-incremental regressions I looked at seem to point to LLVM spending more time, so this may either end up with better optimized code or just make it harder for LLVM to figure things out.

the8472 · 2021-09-18T09:38:18Z

hello-world-check regressed too (especially incr-unchanged), that shouldn't involve llvm.

erikdesjardins · 2021-10-01T06:14:57Z

I tried to reproduce this locally but was unable to. I don't see a regression in wall-time, nor when running under cachegrind. Cachegrind actually shows a slight improvement for encoding-opt.

(For posterity, using e.g. cargo +nightly run --release --bin collector -- diff_local cachegrind --include helloworld --builds Opt --runs IncrUnchanged +3b3ce374d203445eb1d0dce50f4211f4aceb7db6 +20997f6ad81721542e9ef97bb2f58190903a34d8, patched to use --cache-sim=yes --branch-sim=yes)

So, I decided to look at the timeline now that more time has passed and...it seems kinda weird.

Zoomed out, it's not that unusual. This PR introduced a regression and then a later change fixed it--right?

But if we zoom in on the point where wall-time returns to normal, it doesn't make sense:

First, it bounces between slow and fast before settling on fast. Which seems implausible--it's not likely that we introduced a perf improvement, then cancelled it out in the next commit, then reintroduced it three commits later.

Second, the two commits responsible for those two improvements don't make sense:

[A] 38e5764: This is a perf improvement, but it just avoids executing code. So for a >10% decrease in wall-time you'd expect a significant decrease in instruction count, but helloworld-opt only has -0.02%.
[B] 9dd4ce8: This is a cargo update and none of the cargo changes seem related to perf.

(All helloworld and encoding benchmarks exhibit this behavior. And probably other ones but I didn't check.)

I'm not sure what to make of this. Some intermittent issue with the perf collector that makes wall-time measurements longer?

Actually, while typing this up, I think I figured out what it is.

See this comment (on [B] 9dd4ce8) about the cpu governor being reset to ondemand on the perf machine: #88956 (comment)

For any perf triagers noticing the bootstrap and wall time improvement from this PR, it is almost certainly due to enabling performance rather than on demand cpu frequency governor on the perf collector. I think this got disabled by accident at some point (likely after a reboot).

#88308 was merged immediately after this PR. It too had some (unexplainable) wall-time regressions, in await-call-tree, deeply-nested-closures, and deeply-nested-async.

Similarly, if you look at the timeline for those benchmarks:

And zoom in:

The improvements are due to [A] 38e5764 and [C] a58db2e (merged right after [B] 9dd4ce8).

So I think this is what happened:

This PR is merged (20997f6). Halfway though the perf run, the cpu governor gets reset, regressing the wall-time of remaining benchmarks.
Morph layout_raw query into layout_of. #88308 is merged right after (4b9f4b2). It "regresses" the benchmarks that the previous run had already completed* before the governor changed.

...time passes...

[B] 9dd4ce8 hits the perf collector. Halfway through, the cpu governor gets fixed, undoing the regression in some benchmarks.
[C] a58db2e comes right after, undoing it in the rest.

And judging from the timestamp on this comment, [A] 38e5764 was missed initially and manually re-enqueued later. This would explain the downward spike that appears everywhere--although it was committed earlier, its perf run occurred later, after the governor was fixed.

* The regressions attributed to this PR (encoding, helloworld) come (alphabetically) after the regressions attributed to #88308 (await-call-tree, deeply-nested-*), which makes sense because the collector runs benchmarks in alphabetical order. And the same for the perf improvements attributed to [B] and [C].

the8472 · 2021-10-08T17:26:08Z

That seems like a reasonable explanation, marking as triaged.

Use undef for (some) partially-uninit constants There needs to be some limit to avoid perf regressions on large arrays with undef in each element (see comment in the code). Fixes: rust-lang#84565 Original PR: rust-lang#83698 Depends on LLVM 14: rust-lang#93577

rust-highfive assigned estebank Mar 31, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2021

rust-highfive assigned oli-obk and unassigned estebank Mar 31, 2021

oli-obk reviewed Mar 31, 2021

View reviewed changes

compiler/rustc_middle/src/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2021

Dylan-DPC-zz mentioned this pull request Apr 1, 2021

Rollup of 6 pull requests #83750

Closed

Dylan-DPC-zz mentioned this pull request Apr 2, 2021

Rollup of 7 pull requests #83789

Closed

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 2, 2021

lnicola mentioned this pull request Apr 5, 2021

Use arrayvec 0.7 to avoid perf regression in 0.6.1 rust-lang/rust-analyzer#8346

Merged

erikdesjardins mentioned this pull request Apr 6, 2021

Returning MaybeUninit consts generates code to zero out the return value. #83657

Closed

erikdesjardins added 6 commits August 25, 2021 17:49

implement InitMaskCompressed using InitChunkIter, cleanup

3c2b706

add comments

5bef23d

put code in a more logical order

e950f11

improve comments

75fecd5

yet more comment improvements

c07a2eb

use a peekable iterator to check the first chunk

adf3b01

erikdesjardins force-pushed the undefconst branch from d4ab66d to adf3b01 Compare August 25, 2021 22:31

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 26, 2021

bors merged commit 20997f6 into rust-lang:master Aug 26, 2021

rustbot added this to the 1.56.0 milestone Aug 26, 2021

erikdesjardins deleted the undefconst branch August 26, 2021 16:25

SkiFire13 mentioned this pull request Aug 26, 2021

Speed-up waker by using uninitialized array tokio-rs/tokio#4055

Merged

the8472 added the perf-regression Performance regression. label Sep 17, 2021

the8472 added the perf-regression-triaged The performance regression has been triaged. label Oct 8, 2021

erikdesjardins mentioned this pull request Oct 8, 2021

Morph layout_raw query into layout_of. #88308

Merged

erikdesjardins mentioned this pull request Feb 18, 2022

Use undef for (some) partially-uninit constants #94130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use undef for uninitialized bytes in constants #83698

Use undef for uninitialized bytes in constants #83698

erikdesjardins commented Mar 31, 2021 •

edited

Loading

rust-highfive commented Mar 31, 2021

oli-obk commented Mar 31, 2021

oli-obk commented Mar 31, 2021

oli-obk commented Apr 1, 2021

bors commented Apr 1, 2021

erikdesjardins commented Apr 2, 2021

RalfJung commented Apr 2, 2021

bors commented Apr 2, 2021

This comment has been minimized.

bors commented Apr 2, 2021

erikdesjardins commented Apr 6, 2021 •

edited

Loading

nagisa commented Apr 6, 2021 via email •

edited

Loading

RalfJung commented Apr 6, 2021

oli-obk commented Aug 26, 2021

bors commented Aug 26, 2021

bors commented Aug 26, 2021

bors commented Aug 26, 2021

the8472 commented Sep 17, 2021 •

edited

Loading

oli-obk commented Sep 18, 2021

the8472 commented Sep 18, 2021

erikdesjardins commented Oct 1, 2021 •

edited

Loading

the8472 commented Oct 8, 2021

Use undef for uninitialized bytes in constants #83698

Use undef for uninitialized bytes in constants #83698

Conversation

erikdesjardins commented Mar 31, 2021 • edited Loading

rust-highfive commented Mar 31, 2021

oli-obk commented Mar 31, 2021

oli-obk commented Mar 31, 2021

oli-obk commented Apr 1, 2021

bors commented Apr 1, 2021

erikdesjardins commented Apr 2, 2021

RalfJung commented Apr 2, 2021

bors commented Apr 2, 2021

This comment has been minimized.

bors commented Apr 2, 2021

erikdesjardins commented Apr 6, 2021 • edited Loading

nagisa commented Apr 6, 2021 via email • edited Loading

RalfJung commented Apr 6, 2021

oli-obk commented Aug 26, 2021

bors commented Aug 26, 2021

bors commented Aug 26, 2021

bors commented Aug 26, 2021

the8472 commented Sep 17, 2021 • edited Loading

oli-obk commented Sep 18, 2021

the8472 commented Sep 18, 2021

erikdesjardins commented Oct 1, 2021 • edited Loading

the8472 commented Oct 8, 2021

erikdesjardins commented Mar 31, 2021 •

edited

Loading

erikdesjardins commented Apr 6, 2021 •

edited

Loading

nagisa commented Apr 6, 2021 via email •

edited

Loading

the8472 commented Sep 17, 2021 •

edited

Loading

erikdesjardins commented Oct 1, 2021 •

edited

Loading